Switch principal attributes to AttributeMap - #5140
Conversation
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| String PRINCIPAL_ENTITY_ATTRIBUTE_KEY = "org.apache.polaris.core.auth.PRINCIPAL_ENTITY"; |
There was a problem hiding this comment.
Moved to a separate PolarisPrincipalAttributes class.
|
This PR is ready for review. |
There was a problem hiding this comment.
Pull request overview
Updates the PolarisPrincipal attribute model from a raw Map<String, Object> to a type-safe AttributeMap, introducing shared typed keys and propagating the change through core auth/persistence logic, runtime service authentication, and extension authorizer tests.
Changes:
- Switch
PolarisPrincipalto carry anAttributeMapand introducePolarisPrincipalAttributestyped keys for principal entity / role-all / JWT attributes. - Update runtime/service authentication and core resolver/auth preconditions to use typed attribute access.
- Migrate affected tests and extension integrations (OPA, Ranger, Kafka events) to the new attribute model and add coverage for defensive copying.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java | Update test fixture principal construction to use ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/PolarisPersistenceEventListenerTest.java | Use AttributeMap.EMPTY when building principals in event listener tests. |
| runtime/service/src/test/java/org/apache/polaris/service/events/listeners/opentelemetry/OpenTelemetryEventListenerTest.java | Use AttributeMap.EMPTY for metadata user principals. |
| runtime/service/src/test/java/org/apache/polaris/service/events/jsonEventListener/aws/cloudwatch/AwsCloudWatchEventListenerTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/RootPrincipalAugmentor.java | Build root principal attributes using PolarisPrincipalAttributes + ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/PolicyCatalogHandlerAuthzTest.java | Migrate principal attributes to typed AttributeMap in policy authz tests. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/AbstractPolicyCatalogTest.java | Migrate authenticated root principal attributes to ImmutableAttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogTest.java | Use AttributeMap.EMPTY for test principals. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java | Use AttributeMap.EMPTY for handler principal. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerFineGrainedDisabledTest.java | Use PolarisPrincipalAttributes keys in authz-disabled handler tests. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java | Migrate authenticated root principal to typed attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogHandlerAuthzTest.java | Update multiple handler principal constructions to typed attribute maps. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java | Update generic handler authz tests to typed attribute maps. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/AbstractPolarisGenericTableCatalogTest.java | Update generic catalog test base to typed principal attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java | Update assertions and principal creation for AttributeMap + typed keys. |
| runtime/service/src/test/java/org/apache/polaris/service/auth/AuthenticatingAugmentorTest.java | Adjust augmentor test to reflect principal attributes being an AttributeMap. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAuthzTestBase.java | Update authenticated root principal to typed attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceAuthzTest.java | Update admin service authz tests to typed principal attributes. |
| runtime/service/src/test/java/org/apache/polaris/service/admin/ManagementServiceTest.java | Update management service setup principal to typed attribute map. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java | Switch resolved principal attributes from Map to AttributeMap and use typed keys. |
| runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java | Convert principal AttributeMap back into Quarkus SecurityIdentity attributes. |
| polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BaseResolverTest.java | Update resolver test fixture principal creation to typed attributes. |
| polaris-core/src/test/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegrationTest.java | Use AttributeMap.EMPTY for test principal. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/PolarisResolutionManifestLookupKeyTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisPrincipalTest.java | Add test coverage for defensive copy/immutability of principal attributes. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisAuthorizerImplTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationRequestTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationPreConditionsTest.java | Update principal construction and empty-attrs usage for new attribute model. |
| polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java | Update role-all lookup to use typed AttributeMap accessors. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipalAttributes.java | Introduce typed attribute keys for principal-related attributes. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java | Change principal attributes to AttributeMap, remove string-key constants and typed getAttribute helper. |
| polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationPreConditions.java | Update credential-rotation precondition checks to use typed principal attributes. |
| extensions/events/kafka/src/test/java/org/apache/polaris/extensions/events/kafka/KafkaEventListenerTest.java | Replace empty Map principal attributes with AttributeMap.EMPTY. |
| extensions/auth/ranger/src/test/java/org/apache/polaris/extension/auth/ranger/RangerPolarisAuthorizerTest.java | Update Ranger principal deserialization test to use AttributeMap.EMPTY. |
| extensions/auth/ranger/src/main/java/org/apache/polaris/extension/auth/ranger/utils/RangerUtils.java | Update principal-entity lookup to use typed attribute keys. |
| extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerTest.java | Migrate OPA tests to typed attribute maps (including custom user attributes). |
| extensions/auth/opa/src/test/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizerFactoryTest.java | Migrate OPA factory tests to AttributeMap-backed principals. |
flyingImer
left a comment
There was a problem hiding this comment.
I guess I'm a bit late to this topic. I wonder is the intent supporting ABAC authz? do we have any pointer to the design doc that can better navigate me throughout prs?
The intent is not to support ABAC, but just to introduce a type-safe mechanism to retrieve principal attributes. Principal attributes predate this PR. Granted, one could leverage principal attributes to implement some form of ABAC, but this PR does not make any visible step in that direction. |
Thanks Alex for the quick turnaround. In that case, could you please point me or share on why we need this type-safe mechanism change? I noticed #5139, but not quite caught up on why |
Simply to avoid unsafe type casts when retrieving attributes. |
flyingImer
left a comment
There was a problem hiding this comment.
IIUC, IMHO this PR, and the AttributeMap shape from #5139 it's built on, solves the wrong problem for principal attributes.
There are two different kinds of data on the principal today. The first kind: 3 values Polaris itself produces, where Polaris always knows the type ahead of time. That's the resolved PrincipalEntity, the PRINCIPAL_ROLE_ALL boolean, and an optional JWT string. The second kind: an open set of raw attributes copied from the identity provider, where Polaris does not control the keys or the types. AttributeMap treats both kinds the same way, and it doesn't serve either one well.Take the first kind, the 3 Polaris-owned values. PolarisPrincipal is @PolarisImmutable, with exactly one generated implementation, ImmutablePolarisPrincipal. Adding a named method for each of these 3 values costs one interface change plus a rebuild. Nothing else implements this interface, so nothing else needs to change. A named method gives a real compile-time guarantee: get the name wrong, and the build fails. AttributeKey doesn't give that guarantee. It's record AttributeKey(String key), so T plays no part in equals/hashCode. The javadoc says so directly: the type guarantee only holds "as long as callers never create two instances of AttributeKey having the same string key, but different types." That's a rule callers have to remember, not something the compiler checks. Dressing it up as a typed key makes it look safer than a plain Map<String, Object>, when it isn't.
Take the second kind, the raw attributes from the identity provider. L170 in DefaultAuthenticator does new AttributeKey<>(key), and value's static type there is Object, straight from identity.getAttributes(): Map<String, Object>. So every raw attribute gets typed as AttributeKey. That's exactly as safe as a plain string key, since there's no narrower type underneath it to protect.
I also don't think the raw attribute passthrough does anything today. Based on my search, nothing reads an attribute other than the 3 reserved ones off PolarisPrincipal.getAttributes(). So the raw attribute passthrough on PolarisPrincipal looks speculative right now, on top of not doing what it's meant to do.
What I'd suggest instead:
- Give PolarisPrincipal a named accessor for each of the 3 reserved values, instead of AttributeKey constants. @PolarisImmutable makes this cheap here.
- If a future caller genuinely needs the raw identity-provider attributes off PolarisPrincipal, carry them as a plain Map<String, Object> field, not AttributeKey entries. The typed key buys nothing when the value type is always Object.
- If neither set needs a shared string-keyed map, AttributeMap isn't needed for principal attributes at all. That also removes the "who owns this key prefix" question I'd otherwise raise about PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, since there'd be no shared namespace left to collide in.
Let me know if missing anything here
| ImmutableAttributeMap.Builder principalAttributes = ImmutableAttributeMap.builder(); | ||
| identity | ||
| .getAttributes() | ||
| .forEach((key, value) -> principalAttributes.put(new AttributeKey<>(key), value)); |
There was a problem hiding this comment.
IIUC, new AttributeKey<>(key) infers AttributeKey here, since value's static type from identity.getAttributes() is Object. This entry never gets a narrower type than a raw string key would have had. See the general comment for why I think the raw attributes passthrough through AttributeMap isn't earning its keep right now.
| /** | ||
| * Attribute key for the principal entity attribute, of type {@link PrincipalEntity}. | ||
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| public static final AttributeKey<PrincipalEntity> PRINCIPAL_ENTITY_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ENTITY"); | ||
|
|
||
| /** | ||
| * Attribute key, of type {@link Boolean}, used to determine how authorizers should resolve | ||
| * principal roles. | ||
| * | ||
| * <p>When absent or false, authorizers should resolve only the roles returned by {@link | ||
| * PolarisPrincipal#getRoles()}; if an empty set is returned, then no principal roles should be | ||
| * resolved. | ||
| * | ||
| * <p>When present and true, authorizers should resolve all roles granted to the principal at the | ||
| * time of check, ignoring roles returned by {@link PolarisPrincipal#getRoles()} (even if an empty | ||
| * set is returned). | ||
| * | ||
| * <p>This attribute is generally present and true when the principal presented a credential | ||
| * including the pseudo-role {@code PRINCIPAL_ROLE:ALL}. | ||
| * | ||
| * <p>Note: Callers must not assume that this attribute is always present. | ||
| */ | ||
| public static final AttributeKey<Boolean> PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ROLE:ALL"); | ||
|
|
||
| /** | ||
| * Attribute key used to store or retrieve a JSON Web Token (JWT) associated with the {@link | ||
| * PolarisPrincipal}, of type {@link String}. | ||
| * | ||
| * <p>The associated value is expected to represent the JWT provided during authentication and may | ||
| * be used for further validation or for deriving additional claims. | ||
| * | ||
| * <p>Note: callers must never assume that this attribute is present. | ||
| */ | ||
| public static final AttributeKey<String> JWT_ATTRIBUTE_KEY = | ||
| new AttributeKey<>("org.apache.polaris.core.auth.JWT"); |
There was a problem hiding this comment.
Since PolarisPrincipal only has one implementation (@PolarisImmutable), these 3 constants could be named accessors on the interface instead of AttributeKey entries. That gives a real compile-time type guarantee instead of the string-key convention callers of this class currently have to hold to by hand IMO.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)